chore(ci): remove pull_request_target from privileged workflows - #349

Claude finished @Piotr1215's task in 1m 16s —— View job

Review: PR #349

I found two blocking problems. Both are changes that break existing callers without telling them.

1. Advancing backport/v1, as the PR body says to, would silently stop every existing backport (.github/workflows/backport.yaml:79-114)

resolve-event only runs when inputs.pr-event != '' and the event is push or workflow_run. Every downstream job (backport, legacy-classify, link-backports) is gated on needs.resolve-event.outputs.eligible == 'true'. For a current pull_request_target caller that passes no pr-event, resolve-event is skipped, so the whole workflow skips and reports green. The PR body's claim that "existing direct PR callers retain their event behavior" is not true for this file.

The PR body says "advance backport/v1 to the merged commit", but docs/workflows/backport.md says "Publish the workflow as backport/v2; keep backport/v1 unchanged". CLAUDE.md requires the second approach, because callers now need a new trigger, actions: read and a pr-event input. test-backport.yaml only asserts skipped, so CI will not catch the silent no-op. Please fix the rollout steps in the PR body so they cut backport/v2 and leave v1 alone. Alternatively, keep a github.event.pull_request fallback path for callers that pass no pr-event.
Fix this →

2. claude-code-review.yaml now hard-requires github.event_name == 'pull_request' (.github/workflows/claude-code-review.yaml:19-22)

This workflow previously documented pull_request_target as the intended caller trigger. Any caller still on that trigger will now skip review on every PR, including same-repo PRs, with no error. If callers pin @main, they lose review as soon as this merges, before their caller PRs land. Two ways to avoid that:

• Merge the callers first, or ship this as a new version.
• Accept pull_request_target as well when head.repo.full_name == github.repository. That check alone already excludes forks.

3. Minor: the signal path can be replayed (resolve.py:118-160)

For fork PRs, run.pull_requests is empty, so the resolver falls back to looking up PRs by head_sha. It then accepts any merged PR whose head ref and head SHA match the signal run. Someone outside the org can push the head commit of an already-merged public PR to a fork branch with the same name and open a PR. That produces a successful signal run, and the trusted consumer re-runs the backport for the old PR using the PAT. The existing duplicate branch/PR checks limit the damage to wasted runs and possible re-comments. Still, consider also requiring the run's own PR association, i.e. that the signal run's PR number equals the merged PR. For fork PRs that means reading the number from the run's pull_requests field or from /actions/runs/{id}, which gives the PR for same-repo runs.

Not verified

• I did not run the tests or the linters.
• I could not run gh search code here, so I have not confirmed who pins claude-code-review.yaml or backport.yaml@backport/v1. Please check that before tagging.

Claude finished @Piotr1215's task in 1m 24s —— View job

Review: PR #349, re-reviewed after the latest push

The earlier blocking issue with backport.yaml is fixed. The PR body and docs/workflows/backport.md now both say to publish backport/v2 and leave v1 unchanged. Two problems from the last review are still open.

1. Fork-origin replay can still trigger PAT-backed backports for any merged PR (.github/actions/resolve-pr-event/src/resolve.py:235-263)

When the signal run comes from a fork, run.pull_requests is empty. The resolver then falls back to /commits/{head_sha}/pulls, which looks up PRs by commit SHA. It accepts any merged PR whose head.sha and head.ref match the run.

An outside contributor can exploit this:

• Push the public head SHA of any merged PR to a fork branch with the same name.
• Open a PR from that branch. The signal workflow succeeds, and the trusted consumer re-runs the backport for the old PR with gh-access-token.

GitHub only asks for approval on a contributor's first PR, so after that nothing gates this. Duplicate-branch checks and empty cherry-picks keep the damage low. Still, this lets outsiders trigger PAT mutations, so it should be closed.

A cheap fix: also require the run's head_repository.full_name to equal pr.head.repo.full_name. The run object from /actions/runs/{id}/attempts/{n} already contains head_repository. Add a test for the replay case.

Fix this →

2. claude-code-review.yaml silently stops reviewing for existing pull_request_target callers (.github/workflows/claude-code-review.yaml:27-30)

The new guard requires github.event_name == 'pull_request'. Callers following the old documented trigger (pull_request_target) will skip every PR, same-repo PRs included, and the run still shows green. This workflow has no versioned tag, so the break reaches those callers as soon as this merges. Either option fixes it:

• Accept pull_request_target as well. The head.repo.full_name == github.repository check already keeps forks out, and once forks are excluded pull_request_target gives no extra access.
• Migrate the callers before this merges.

Fix this →

Not verified

• I did not run the tests or linters.
• I did not run gh search code, so I don't know which repos pin claude-code-review.yaml.
• I could not check whether the runner passes NODE_OPTIONS=--require=... from step env through to sorenlouv/backport-github-action. The preload test runs locally and does not exercise the runner. A real run is needed to confirm it.

Claude finished @Piotr1215's task in 1m 14s —— View job

Review: PR #349, re-reviewed after 2a987c3

Both open items from my last review are fixed:

• The resolver now checks that the run's head_repository matches pr.head.repo (resolve.py:244-246). A fork can no longer replay a same-repo PR.
• claude-code-review.yaml accepts pull_request_target for same-repo PRs again.

I found two new problems.

1. The documented caller setup runs the backport twice on every labeled merge (.github/actions/resolve-pr-event/README.md, docs/workflows/backport.md)

The example signal workflow uses types: [closed, labeled], and the example consumer listens to both push and workflow_run. When a PR that already carries a backport-to-* label is merged, two runs start:

• The push to main resolves the PR from the commit range.
• The closed signal run completes, and workflow_run resolves the same PR.

Both runs pass every check, so backport runs twice for the same PR, and so do fix-imports, legacy-backport and link-backports. The concurrency group runs them one after the other, not at the same time. I have not confirmed what the second run does: either sorenlouv finds the existing backport branch or PR and posts a failure comment on the source PR, or it does nothing. Either way it wastes a PAT run.

closed is not needed. The push path already covers merges, including ones where the signal was skipped. The resolver already rejects labeled events on PRs that are not merged yet. Change the documented signal to types: [labeled] only.

Fix this →

2. On push, restoring an event repeats the whole commit-range walk for each PR, which can hit the GITHUB_TOKEN rate limit (resolve.py:204-209, backport.yaml resolve-event and backport jobs)

Every call to resolve() on a push makes one /commits/{sha}/pulls call for each commit in the range. Restoring with event-json calls resolve() again and only filters afterwards. The walk therefore runs:

• once in select,
• then twice in each matrix leg (once in resolve-event, once in backport).

That comes to about commits × (1 + 2 × PRs) requests, plus the PR lookups. Two examples:

• A rebase-merged PR with 60 commits costs about 180 calls.
• A sync merge of 200 commits that carries 20 PRs costs about 8,200 calls.

GITHUB_TOKEN allows 1,000 requests per hour per repository. When the limit is hit, the HTTP 403 raises RuntimeError, so the job fails and the backports are lost.

When event-json is present, the resolver should skip the range walk:

1. Fetch /pulls/{number}.
2. Check it with merged_pr.
3. Check ancestry with the same compare(merge_commit_sha...default) already used on the workflow_run path.

Fix this →

Not verified

• I did not run the tests or linters.
• I have not confirmed that a step-level NODE_OPTIONS reaches sorenlouv/backport-github-action on a real runner.
• I have not confirmed what sorenlouv does on a duplicate run for a backport that already exists.

Claude finished @Piotr1215's task in 46s —— View job

Review: PR #349, re-reviewed after a770a9d

Both items from my last review are fixed:

• The documented signal workflow now uses types: [labeled] only, so a labeled merge no longer runs the backport twice.
• When restoring a push event, the resolver no longer walks the commit range. It makes one repo call, one PR call and one ancestry call.

I found one remaining problem. It is minor.

On push, the restore path no longer checks that the PR belongs to the push being processed (.github/actions/resolve-pr-event/src/resolve.py:114-117, 144, 160, 167-170)

When requested_number is set, commits stays empty. That has two effects:

• The check pr['merge_commit_sha'] not in commits at line 160 is skipped.
• The only remaining check is that the merge commit is an ancestor of the default branch.

So during a push run, resolve-event and backport will accept any pr-event that names any PR ever merged to the default branch. They will then run sorenlouv with the PAT for that PR, even though the push that triggered the run did not contain it.

Today pr-event comes from the caller's own default-branch select job, so this is not an outside attack. But it weakens the PR's stated guarantee of a recheck "before backport jobs receive mutation tokens". It also means a caller that wires pr-event up wrong, for example from a stale output or a hard-coded value, re-backports old PRs with no error.

You can restore the check and keep the O(1) cost with two compare calls in place of the range walk:

1. /compare/{before}...{merge_commit_sha} must return ahead.
2. /compare/{merge_commit_sha}...{after} must return ahead or identical.

This replaces the current ancestry check against the default branch on the push path, and the request-count test still holds.

Fix this →

Not verified

• I could not run the tests or the linters, because the Bash permissions here do not allow them.
• I have not confirmed on a real runner that the step-level NODE_OPTIONS reaches sorenlouv/backport-github-action. I raised this before and it is still open.

Claude finished @Piotr1215's task in 34s —— View job

Review: PR #349, re-reviewed after 28f0433

I found no new bugs or security problems in this push, and the item from my last review is fixed.

The push restore path now ties the restored PR to the push being processed (.github/actions/resolve-pr-event/src/resolve.py:171-177). I checked the compare semantics:

• compare(merge...before) must return behind or diverged. That means before does not already contain the merge commit.
• compare(merge...after) must return ahead or identical. That means after contains it.

So a stale or hard-coded pr-event now gets rejected. Allowing diverged is correct for side-branch merges brought in by a sync merge. Pushes with before set to zeros, and force pushes, are still rejected earlier at line 112, so before is always a real SHA here. The new tests cover each boundary status in both directions, and the restore path still makes a fixed number of API calls.

One small correction: the PR body still says restoration takes "three API requests". It now takes five (repo, PR, default-branch ancestry, and the two range compares). The cost is still constant, so only the number in the description needs updating.

Still not verified

• I could not run the tests or linters, because the Bash permissions here do not allow python3/pytest.
• I have not confirmed on a real runner that the step-level NODE_OPTIONS reaches sorenlouv/backport-github-action. This has been open since earlier reviews, and only a live caller run can settle it.

Claude finished @Piotr1215's task in 41s —— View job

Review: PR #349, re-reviewed after 6932486

The changes for @vcauesantos's review comments are in. The pagination cap and an incomplete range now fail the run instead of passing with a warning. claude-code-review.yaml now names pull_request_target directly, and the test allowlists it explicitly. The dead issues branch is gone from claude.yaml, but it is still in claude.yml:35.

I found one new problem.

An unassociated commit stops backports for every other PR in the same push, and a re-run cannot recover them (.github/actions/resolve-pr-event/src/resolve.py:146-154)

The new RuntimeError is raised inside the commit loop, before write_outputs. If one commit in the range has no PR association, the whole select step exits 1 and no PR in the push gets backported. Two common cases trigger this:

• A bot pushes straight to the default branch, for example a version bump or a generated-docs commit. Every one of these pushes now turns the backport workflow red on main.
• A sync merge or a batched push that contains a PR merge plus one direct commit. The labeled PR is dropped along with the direct commit.

A re-run gives the same result, because a direct commit never gets an association. The README tells maintainers to "investigate those runs before retrying", but no retry can pick up the PRs that were dropped. The change is meant to surface missed backports, but in this case it causes them.

The fix: collect the unassociated SHAs during the loop instead of raising. Write outputs for the PRs that did resolve so their matrix legs still run. Then report the missing SHAs. Either exit 1 after write_outputs (and let the select job's downstream if accept a failed select that still has eligible == 'true'), or emit a ::warning:: listing them. Add a test with a push range of one merged PR plus one direct commit, and assert that the PR is still emitted.

Fix this →

Not verified

• I did not run the tests or linters.
• It is still open whether a step-level NODE_OPTIONS reaches sorenlouv/backport-github-action. Only a live caller run can confirm it.

Claude finished @Piotr1215's task in 36s —— View job

Review: PR #349, re-reviewed after 90cf9c0

90cf9c0 changes only a test. The privileged-trigger scan now covers just .github/workflows/, and claude-code-review.yaml is still the only exception it allows. That is fine.

The blocking issue from my last review is still open. I found nothing else new.

1. A commit with no PR association still fails the whole push, and every other PR in that push loses its backport (.github/actions/resolve-pr-event/src/resolve.py:146-154)

The loop now retries the association lookup up to 3 times, 1s apart. After that it still raises RuntimeError inside the loop, before write_outputs. main() returns 1, and the outputs are never written.

• A direct or bot commit on the default branch, such as a version bump or a generated-docs commit, will never get an association. Every such push turns red, and re-running cannot fix it.
• If the same push also merges a labeled PR, for example in a sync merge or a batched push, that PR's backport is dropped too. The README asks maintainers to "investigate those runs before retrying", but no retry can recover the dropped backport.
• The docs disagree with the code. docs/workflows/backport.md:31 still says "A direct push skips", while the code fails.

The retries help with association lag, which is the real race. Collect the SHAs that still have no association instead of raising. Write outputs for the PRs that did resolve. Then report the missing SHAs, either as ::warning:: or by exiting non-zero after outputs are written, with the downstream if loosened to still run when eligible == 'true'. Add a test with one merged PR plus one direct commit, and assert that the PR is still emitted.

Fix this →

Not verified

• I did not run the tests or the linters.
• I have not confirmed on a real runner that a step-level NODE_OPTIONS reaches sorenlouv/backport-github-action. Only a live caller run can settle that.

Claude finished @Piotr1215's task in 21s —— View job

Review: PR #349, re-reviewed after f5d40f5

The blocking item from my last review is fixed, and I found no new bugs, security problems or performance problems.

When a commit has no PR association, the push no longer fails (.github/actions/resolve-pr-event/src/resolve.py:106-129, :275-294):

• associated_numbers collects those commits and retries them as one batch, after 2s and then 4s. The wait stays at 6s in total no matter how many direct commits the push has, and commits that already resolved are not looked up again.
• write_outputs runs first, and the ::warning:: comes after it. PRs merged in the same push still get their matrix legs, and a direct or bot push no longer turns the run red.
• The restore path (requested_number set) leaves commits empty, so it never enters this loop. Its request count stays the same as before.
• An incomplete compare range and the pagination cap still raise, as intended.
• The new tests cover a push with one direct commit and one merged PR, the batched retry timing, the order of outputs and warning, and the limit on how many commits the warning lists.
• docs/workflows/backport.md and the README now describe this behaviour.

Not verified

• I could not run the tests. Bash permission was denied for pytest.
• It is still open whether a step-level NODE_OPTIONS reaches sorenlouv/backport-github-action. Only a live caller run can confirm it.

Addressed in f5d40f5: a pushed commit with no PR no longer fails the run or drops the other PRs in that push.

• resolve.py collects the commits that still have no PR after retries, writes outputs for the PRs it did resolve, then prints a ::warning:: listing those commits and the remedy: re-add the backport-to- label to start the backport through the signal. A direct or bot push stays green, and a labeled PR merged in the same push still gets its matrix leg.
• Retries run as one batch after 2 and 4 seconds, so a push with many direct commits waits 6 seconds in total rather than per commit.
• The 1,000-entry pagination cap and an incomplete compare range still fail the run.
• docs/workflows/backport.md and the action README describe the warning instead of a failure.
• New tests: one merged PR plus one direct commit still emits the PR through main() with exit 0 and the warning; unassociated commits retry as one batch; the warning caps the listed SHAs. make test-resolve-pr-event (91 passed), make lint with zizmor 1.30.1 and make check-docs pass locally.

@vcauesantos this changes the answer to your question on resolve.py:146. An empty lookup is now retried, and a commit that still has no PR is listed in a warning with its remedy. Failing could not tell a direct push from association lag, and it dropped every other PR in the push. I re-requested your review because you approved the failing version.